DATAJDBC-326 - Support conversion of backreferences. #118
Conversation
| /** | ||
| * @author Jens Schauder | ||
| */ | ||
| public class ParentKeys { |
There was a problem hiding this comment.
Let's turn this type into a proper value/domain type such as Identifier as in entity/aggregate identifier that encapsulates simple and composite (primary) key parts and move the type into data.relational.domain.
This seems a decent addition that could be reused in Spring Data R2DBC.
| * | ||
| * @param additionalParameters | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
We should not introduce @Deprecated methods in newly introduced code.
| /** | ||
| * Creates ParentKeys with backreference for the given path and value of the parents id. | ||
| */ | ||
| static ParentKeys forBackReferences(PersistentPropertyPath<RelationalPersistentProperty> path, @Nullable Object value) { |
There was a problem hiding this comment.
Let's move these factory/construction method to BasicJdbcConverter.
| String name; | ||
| Object value; | ||
| Class<?> targetType; | ||
| } |
There was a problem hiding this comment.
Proposal: We might want to consider a ParentKeyConsumer (interface ParentKeyConsumer{ void accept(String,Object,Class<?>); }) to expose a forEach(ParentKeyConsumer) method to consume the object.
There was a problem hiding this comment.
I created a forEach method but no separate consumer interface since I currently don't see a benefit of the generic Consumer<ParentKey>.
|
@mp911de the second round of review, please. |
schauder
force-pushed
the
issue/DATAJDBC-326
branch
from
ed125c2 to
76dffdd
Compare
| this.keys = Collections.unmodifiableList(keys); | ||
| } | ||
|
|
||
| public Identifier withQualifier(PersistentPropertyPath<RelationalPersistentProperty> path, Object value) { |
There was a problem hiding this comment.
Let's move the method to a place where it is called as we should not have references to PersistentPropertyPath in the domain model.
There was a problem hiding this comment.
That would put the method in the DefaultJdbcInterpreter.
This doesn't feel right to me for two reasons:
- We now have factory methods in one place and add-functionality in a different place.
- One keeps passing
Identifierinstances around.
I wonder if a JdbcIdentifierBuilder in the jdbc.core.convert package would be a better approach.
What do you think?
There was a problem hiding this comment.
If we don't have a good place to build Identifier, then let's create one. JdbcIdentifierBuilder sounds good.
| * @since 1.1 | ||
| */ | ||
| @Value | ||
| public static class SingleIdentifierValue { |
There was a problem hiding this comment.
How about renaming it to Qualifier? This would align nicely with the withQualifier(…) method.
There was a problem hiding this comment.
But it isn't just used for qualifiers, but also for backreferences and in the future simply for parts of an identifier.
While I'm not proud of the current name I don't think "qualifier" would be an improvement.
|
|
||
| private final List<SingleIdentifierValue> keys; | ||
|
|
||
| public Identifier(List<SingleIdentifierValue> keys) { |
There was a problem hiding this comment.
Let's make this one private and introduce rather factory methods (empty(), from(Map)).
There was a problem hiding this comment.
So you didn't want to move fromNamedValues out to BasicJdbcConverter?
|
|
||
| @Deprecated | ||
| public Map<String, Object> getParametersByName() { | ||
| return new HashMap<>(); |
There was a problem hiding this comment.
Is this intended to return an empty map?
| } | ||
|
|
||
| public Collection<SingleIdentifierValue> getParameters() { | ||
| return keys; |
There was a problem hiding this comment.
Returning the collection directly breaks immutability.
There was a problem hiding this comment.
keys is already an unmodifiableList so the only breakage of immutability I see is when contained values aren't immutable and I don't think we can do anything about that, can we?
| * @author Jens Schauder | ||
| * @since 1.1 | ||
| */ | ||
| public class Identifier { |
There was a problem hiding this comment.
Let's make the type final to encapsulate it.
|
|
||
| Identifier identifier = BasicJdbcConverter // | ||
| .forBackReferences(path, "parent-eins") // | ||
| .withQualifier(path, "list-index-eins"); |
| public static Identifier fromNamedValues(Map<String, Object> additionalParameters) { | ||
|
|
||
| List<Identifier.SingleIdentifierValue> keys = new ArrayList<>(); | ||
| additionalParameters.forEach((k, v) -> keys.add(new Identifier.SingleIdentifierValue(k, v, v == null ? Object.class : v.getClass()))); |
There was a problem hiding this comment.
By using factory/mutator methods on Identifier, there should not be a need to instantiate SingleIdentifierValue from within the code.
| * | ||
| * @param additionalParameters | ||
| */ | ||
| public static Identifier fromNamedValues(Map<String, Object> additionalParameters) { |
There was a problem hiding this comment.
Let's rename the method to createIdentifier(…).
Ids used as backreferences now get properly converted. Introduced ParentKeys. ParentKeys hold information about data that needs to be considered for inserts or updates but is not part of the entity. Appart from column names and values they also hold information about the desired JdbcType in order to facilitate conversions. This replaces the Map handed around in the past.
ParentKeys gets renamed to Identifier and moved to spring-data-relational. This facilitates reuse beyond just references to parents. The factory methods moved to BasicJdbcConverter.
Renamed test class to match default naming pattern for unit tests.
Variables renamed to match changed class names
schauder
force-pushed
the
issue/DATAJDBC-326
branch
from
76dffdd to
daa9943
Compare
|
Tried to apply the requested changes, except where commented above. |
schauder
force-pushed
the
issue/DATAJDBC-326
branch
from
daa9943 to
aaa894f
Compare
|
Just force-pushed the next try. I hope I didn't miss the point again. |
Add Javadoc. Tweak naming and factory methods. Add equals/hashcode to Identifier. Extend tests.
Ids used as backreferences now get properly converted. Introduced Identifier to hold information about data that needs to be considered for inserts or updates but is not part of the entity. Apart from column names and values they also hold information about the desired JdbcType in order to facilitate conversions. This replaces the Map handed around in the past. Original pull request: #118.
Add Javadoc. Tweak naming and factory methods. Add equals/hashcode to Identifier. Extend tests. Original pull request: #118.
|
That's merged and polished now. |
Ids used as backreferences now get properly converted. Introduced Identifier to hold information about data that needs to be considered for inserts or updates but is not part of the entity. Apart from column names and values they also hold information about the desired JdbcType in order to facilitate conversions. This replaces the Map handed around in the past. Original pull request: spring-projects#118.
Add Javadoc. Tweak naming and factory methods. Add equals/hashcode to Identifier. Extend tests. Original pull request: spring-projects#118.
Ids used as backreferences now get properly converted.
Introduced ParentKeys.
ParentKeys hold information about data that needs to be considered for inserts or updates but is not part of the entity.
Appart from column names and values they also hold information about the desired JdbcType in order to facilitate conversions.
This replaces the Map handed around in the past.